Implement Offset Algorithm #935

thehappycheese · 2022-11-18T04:51:52Z

I agree to follow the project's code of conduct.
I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Defines an OffsetCurve trait and implements the associated function .offset_curve(distance) for some geometry types. The offset_curve function offsets the edges of a geometry by a specified distance, where the sign of distance determines if the edges of the geometry are offset to the 'left' or 'right'.

'Offset curve' is like a 'one-sided buffer' and, unlike buffer, it is only meaningful for some geometry types. The table below summarises the different outputs.

A 'clipped offset curve' has had parts of the output removed to ensure that all edges in the output are at least distance from the input, even if the input is self-overlapping. So far, this clipping step has not been implemented.

The following table is a work in progress:

Input	Offset Curve	Offset Curve - Clipped	Buffer
`Point`	Undefined	Undefined	Circle (`Polygon`)
`Line`	`Line`	`Line`	Capsule (`Polygon`)
`LineString`	`LineString` (possibly self overlapping)	`MultiLineString`	Sausage (`Polygon`)
`Polygon`	`Option<Polygon>` (always closed, but possibly malformed)	`Option<Polygon>` (possibly unclosed, possibly malformed)	`Polygon`
...	...	...	...

GEOS and JTS

Choice of Algorithm

Issue Geometry buffering #641 suggests a more advanced algorithm (Voronoi-based straight skeleton approach which is meant for buffering, but might also work for offset_curve)
Issue Support (Convex) Minkowski Sum construction #883 suggests a Minkowski Sum which is a more powerful type of buffering operation

I am not sure if the algorithm I am trying to implement here is any better or worse than those, but it is one I have got working in the past in python so I figure I have the best chance of success with it. It loosely follows the algorithm described by
Xu-Zheng Liu, Jun-Hai Yong, Guo-Qin Zheng, Jia-Guang Sun. An offset algorithm for polyline curves. Computers in Industry, Elsevier, 2007, 15p. inria-00518005

Trait Implementations

For coordinate types

where T:CoordFloat
where T:CoordNum (??? seems tricky)

Note:
Offset for Point Can't be defined without confusion.
The user may expect the result to be one of the following:

Return the input unchanged,

a translation (but in what direction?)

a circle (like a buffer, but as previously explained,
the offset operation is different to the buffer operation).

an empty LineString (weird, yes, but this is actually what GEOS does 🤔)

Issues to Fix:

Change name from Offset to OffsetCurve to match terminology in GEOS
and jts
Match offset sign / direction with GEOS and JTS if possible
Make proper use of SimpleKernel / RobustKernel
Prevent execution when the specified offset distance is zero
Mitre-limit is implemented so that a LineString which doubles-back on
itself will not produce an elbow at infinity
- Make Mitre Limit configurable?
Option to clip output such that non-adjacent line segments in the output
are not self-intersecting.

Hopefully I have started off my contribution in the right direction? :)

Many thanks for your time

urschrei

Hi @thehappycheese, welcome and thanks for the contribution! I've left a load of review comments – please don't feel that you need to address them all at once. In particular, it looks like you're having some trouble using the Kernel trait, and this might be a good place to start. I and other contributors are happy to help here, and we can do it inline in the PR, and / or on Discord: https://discord.gg/Fp2aape

urschrei · 2022-11-18T16:24:07Z

geo/src/algorithm/offset/line_intersection.rs

+    // TODO: I'm still confused about how to use Kernel / RobustKernel;
+    //       the following did not work. I need to read more code
+    //       from the rest of this repo to understand.
+    // if Kernel::orient2d(*a, *b, *d) == Orientation::Collinear {


Orientation is an enum, so you have to use a match for control flow.

something like

match Kernel::orient2d(*a, *b, *c) { Orientation::Collinear => { // keep going } _ => { // not collinear, bail out or test different combination?? } }

As you note, the use of arbitrary "magic" threshold values without considerable justification and accompanying comments explaining the reasoning behind it for future maintainers is unlikely to be accepted (I've just finished removing an algorithm containing a lot of these that I had the misfortune of writing myself…), so if we can use existing mechanisms that would be much better.

urschrei · 2022-11-18T16:27:21Z

geo/src/algorithm/offset/cross_product.rs

@@ -0,0 +1,85 @@
+use crate::CoordFloat;


I'm a bit confused about this. Would the dot product sign, i.e. the Orientation suffice? It looks like you're just checking whether the result is -1, 1, or 0…

Apologies, looks like my inept git-fu has disconnected all your comments from the code 😕 I think maybe re-basing the branch on my fork was the wrong thing to do.

cross_product simplifies line_segment_intersection_with_parameters(): While it is used for a collinear check at the start of that function (which could be replaced with Kernel::dot_product_sign or Kernel::orient2d) it is used twice more further along to calculate the parameters t_ab and t_cd so I do not believe it can be substituted for an orientation check.

let t_ab = cross_product_2d(ac, cd) / ab_cross_cd; let t_cd = -cross_product_2d(ab, ac) / ab_cross_cd;

line_segment_intersection_with_parameters() is currently only used via a wrapper function line_segment_intersection_with_relationships(); in that function it checks each t_ab and t_cd to find which of three ranges they lie in:

less than zero

between zero and one

greater than one

It's possible there is some tricky way to express this check in terms of orient2d(), I'll have a think about it.

It does feel weird to have cross_product() out in it's own module. In the past I have worked with libraries that have a Vector2 type where this kind of function would be implemented. It is simple enough to be inline perhaps, but I wrote up all the tests and documentation because in the past I have made many mistakes with putting negatives in the wrong place and if you make this kind of mistake an even number of times you might not realize since it cancels out 😋

urschrei · 2022-11-18T16:28:16Z

geo/CHANGES.md

@@ -11,6 +11,7 @@
  * <https://github.com/georust/geo/pull/928>
 * Added `RemoveRepeatedPoints` trait allowing the removal of (consecutive)
  repeated points.
+* Added very basic `Offset` trait and implemented it for `Line` and `LineString`


A minor nit: there's no need to qualify this as "very basic", since the docs will make the limitations clear.

👍 I will try to reword it to be more specific, or be more clear about any limitations in the rest of the docs

urschrei · 2022-11-18T16:35:49Z